-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add per entry and per property meta data field. #463
Add per entry and per property meta data field. #463
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for putting together this PR. I like the idea of introducing the metadata mechanism now and specifying the actual metadata properties later.
… to Database-Provider-Specific fields.
Code suggestions from Vaitkus. Co-authored-by: Antanas Vaitkus <[email protected]>
…med them to Database-Provider-Specific fields." This reverts commit 54b632c.
… for metadata fields." This reverts commit d343a86.
…IMADE into JPBergsma_add_meta # Conflicts: # optimade.rst
…ds to section database-provider-specific-properties.
During the last online optimade meeting, the point was raised that we should also define how the metadata fields should be defined in the property definitions. I expect that some metadata subfields occur in several metadata properties with the same meaning. For example, many numerical properties may have a confidence interval. For now, I have specified that these should have the same $id field. (I hope that this was indeed the intention of the original $id field) but should otherwise be declared repeatedly. I have also added a metadata_for field to the metadata dictionary, as asked by @sauliusg. I do not think we reached a consensus on whether this field should be added, so I can easily remove it if that is what you prefer. Do we want to return only the field name or the complete path including the field name in the metadata_for field? |
As far as I can see, we still don't have a solution to the concerns of some people in the last meeting that it is an issue to limit the field name namespace; i.e., we really should use a different meta name delimiter? (@sauliusg alternatively argued to reduce the |
Co-authored-by: Rickard Armiento <[email protected]>
…IMADE into JPBergsma_add_meta
…he property definition itself.
I would once again advise against assigning special meanings or special requirements to names. IMHO this is a technological debt, and a pretty nasty one. Special names pollute the name space, and if we go that way and try to apply it universally, we might very soon arrive at contradictory requirements. We were bitten several times by a decision to use "special" names, do I am wary about such choices. If a data element "is_metadata_for" is embedded into the data response, what is a problem to test it and get the necessary information during processing? |
Is anyone working on resolving the conflict? If not, I can give it a stab. |
@merkys Sorry, didn't see your comment. Already did the merge - it was a bit nasty, but not too bad. I'm happy with this PR as it now is. There is apparently some disagreement over what the key inside "meta" should be to communicate metadata for properties, |
No problem, I wanted to get an ACK before embarking on it anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I re-approve the proposal. I prefer the current key name property_metadata
.
Co-authored-by: Antanas Vaitkus <[email protected]>
During the discussion on ranged properties, we thought it would be a good idea to have a method for storing per entry metadata for a property. This has been discussed in Issue #410 and #462.
In this PR, I have added a brief description for this metadata field, so I can use it for the ranged properties in PR #452.
I explicitly allow metadata fields to also have metadata of its own.
When a ranged property would have uncertainty values. That property would be just as large as the original ranged property, so I think it makes sense to allow metadata fields to have metadata themselves.
We can discuss this further during the optimade meeting of next Friday.